Conversation
11-opengraph/src/entry.py
Outdated
|
|
||
| # Remove existing OpenGraph and Twitter meta tags to avoid duplicates | ||
| meta_remover = ExistingMetaRemover() | ||
| rewriter.on('meta[property^="og:"]', to_js(meta_remover)) |
There was a problem hiding this comment.
Without to_js here we get an unclear error:
✘ [ERROR] Uncaught Error: This borrowed attribute proxy was automatically destroyed in the process of destroying the proxy it was borrowed from. Try using the 'copy' method.
For more information about the cause of this error, use `pyodide.setDebug(true)` Error
at _getAttrs (pyodide-internal:generated/emscriptenSetup:22274:19)
at _adjustArgs (pyodide-internal:generated/emscriptenSetup:22292:77)
at apply (pyodide-internal:generated/emscriptenSetup:23240:37)
at apply (pyodide-internal:generated/emscriptenSetup:23083:20)
✘ [ERROR] Uncaught Error: This borrowed attribute proxy was automatically destroyed in the process of destroying the proxy it was borrowed from. Try using the 'copy' method.
For more information about the cause of this error, use `pyodide.setDebug(true)`
✘ [ERROR] Uncaught Error: This borrowed attribute proxy was automatically destroyed in the process of destroying the proxy it was borrowed from. Try using the 'copy' method.
For more information about the cause of this error, use `pyodide.setDebug(true)`
After reading pyodide/pyodide#1855 I was able to deduce that to_js is at least the first thing I need to do in order to then be able to copy as well. Looks like just doing to_js works here, but maybe there is a hidden bug here that I'm not aware of.
Anyway, we should put HTMLRewriter into our SDK to make this easier for devs.
There was a problem hiding this comment.
create_proxy would also work, but I found another example using to_js and it is probably good to be consistent.
There was a problem hiding this comment.
I'd say it should be create_proxy here.
There was a problem hiding this comment.
To get the resource management right, it should probably be:
meta_remover = create_proxy(ExistingMetaRemover())
meta_injector = create_proxy(MetaTagInjector(og_data))
try:
# Create an HTMLRewriter instance
rewriter = HTMLRewriter.new()
rewriter.on('meta[property^="og:"]', meta_remover)
rewriter.on("head", to_js(meta_injector))
return rewriter.transform(response.js_object)
finally:
meta_remover.destroy()
meta_injector.destroy()We should probably add a wrapper for HTMLRewriter to the sdk that can manage the handler lifetimes.
There was a problem hiding this comment.
Okay, done. I needed to add a few copy in there too. Hopefully that makes sense.
ryanking13
left a comment
ryanking13
left a comment
There was a problem hiding this comment.
CC @ryanking13 (I can't add you as a reviewer yet)
Yeah, do you know how I can be added to the GitHub org?
11-opengraph/src/entry.py
Outdated
|
|
||
| # Remove existing OpenGraph and Twitter meta tags to avoid duplicates | ||
| meta_remover = ExistingMetaRemover() | ||
| rewriter.on('meta[property^="og:"]', to_js(meta_remover)) |
There was a problem hiding this comment.
create_proxy would also work, but I found another example using to_js and it is probably good to be consistent.
dom96
force-pushed
the
dominik/opengraph
branch
2 times, most recently
from
08c0160 to
eb78296
Compare
11-opengraph/src/entry.py
Outdated
| rewriter.on('meta[property^="og:"]', meta_remover.copy()) | ||
| rewriter.on('meta[name^="twitter:"]', meta_remover.copy()) | ||
| # Inject new OpenGraph meta tags into the <head> element | ||
| rewriter.on("head", meta_injector.copy()) |
There was a problem hiding this comment.
I think we don't need these copies.
There was a problem hiding this comment.
it fails without them
There was a problem hiding this comment.
Does the rewriter do any work after rewriter.transform returns?
There was a problem hiding this comment.
I wouldn't think that it does, but I guess the error implies this
I removed the copy() and the destroy() for now
dom96
force-pushed
the
dominik/opengraph
branch
2 times, most recently
from
0476c1b to
16c9b20
Compare
dom96
force-pushed
the
dominik/opengraph
branch
from
16c9b20 to
7592ede
Compare
3 participants
Uses HTMLRewriter to inject meta tags
Deployed to: https://python-opengraph.runtime-playground.workers.dev/
CC @ryanking13 (I can't add you as a reviewer yet)